Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

upcoming: [DI-23083] - Migrated CloudPulseTimeRangeSelect to DateTimeRangePicker #11573

Open
wants to merge 38 commits into
base: develop
Choose a base branch
from

Conversation

nikhagra-akamai
Copy link
Contributor

@nikhagra-akamai nikhagra-akamai commented Jan 28, 2025

Description 📝

Migrated Time range select component in ACLP to Date time range picker with presets component.

Changes 🔄

List any change(s) relevant to the reviewer.

  1. Modified DateTimePicker.tsx to avoid user to type inside time select.
  2. Modified DateTimeRangePicker.tsx to have 1hr option in presets and also set seconds value to 0 for the selected dates.
  3. Updated GlobalFilters & CloudPulseDashboardWithFilters to use CloudPulseDateTimeRangePicker instead of CloudPulseTimeRangeSelect
  4. Updated metrics call request body to use absolute_time_duration for custom date, 'this month' & 'last month' presets & relative_time_duration for rest of the other presets.

Target release date 🗓️

10th February 2025

Preview 📷

Include a screenshot or screen recording of the change.

🔒 Use the Mask Sensitive Data setting for security.

💡 Use <video src="" /> tag when including recordings in table.

Before After
Screenshot 2025-01-28 at 5 24 03 PM Screenshot 2025-01-28 at 5 24 46 PM
Screenshot 2025-01-28 at 5 25 47 PM Screenshot 2025-01-28 at 5 25 53 PM
Screenshot 2025-01-28 at 5 28 08 PM Screenshot 2025-01-28 at 5 28 20 PM
Screenshot 2025-01-28 at 5 29 37 PM Screenshot 2025-01-28 at 5 29 44 PM

How to test 🧪

  1. Click on monitor option in mega menu or switch to mock user if you can't access directly
  2. From time range drop down select any preset or click custom to open date range picker
  3. Play around with various combination of values.
  4. If you are testing without mock, then by reloading the page you can notice previous selection are there in time range component.

Prerequisites

(How to setup test environment)

  • ...
  • ...

Reproduction steps

(How to reproduce the issue, if applicable)

  • ...
  • ...

Verification steps

(How to verify changes)

  • ...
  • ...
Author Checklists

As an Author, to speed up the review process, I considered 🤔

👀 Doing a self review
❔ Our contribution guidelines
🤏 Splitting feature into small PRs
➕ Adding a changeset
🧪 Providing/improving test coverage
🔐 Removing all sensitive information from the code and PR description
🚩 Using a feature flag to protect the release
👣 Providing comprehensive reproduction steps
📑 Providing or updating our documentation
🕛 Scheduling a pair reviewing session
📱 Providing mobile support
♿ Providing accessibility support


  • I have read and considered all applicable items listed above.

As an Author, before moving this PR from Draft to Open, I confirmed ✅

  • All unit tests are passing
  • TypeScript compilation succeeded without errors
  • Code passes all linting rules

@nikhagra-akamai nikhagra-akamai changed the title Date range picker pr upcoming: [DI-23083] - Migrated CloudPulseTimeRangeSelect to DateTimeRangePicker Jan 28, 2025
@nikhagra-akamai nikhagra-akamai self-assigned this Jan 28, 2025
@nikhagra-akamai nikhagra-akamai marked this pull request as ready for review January 28, 2025 12:05
@nikhagra-akamai nikhagra-akamai requested a review from a team as a code owner January 28, 2025 12:05
@nikhagra-akamai nikhagra-akamai requested review from carrillo-erik and abailly-akamai and removed request for a team January 28, 2025 12:05
@@ -249,6 +248,9 @@ export const DateTimePicker = ({
padding: 0,
}),
},
field: {
readOnly: true,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to avoid user to type inside time select otherwise it'll break the UI

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why?

handleTimeDurationChange(timerDuration);
handleTimeDurationChange({
...timeDuration,
end: convertToGmt(timeDuration.end),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

converting to gmt because api request accepts date in gmt format

Copy link

github-actions bot commented Jan 28, 2025

Coverage Report:
Base Coverage: 79.11%
Current Coverage: 79.11%

@linode-gh-bot
Copy link
Collaborator

Cloud Manager UI test results

🎉 488 passing tests on test run #4 ↗︎

❌ Failing✅ Passing↪️ Skipped🕐 Duration
0 Failing488 Passing2 Skipped97m 55s

Copy link
Contributor

@abailly-akamai abailly-akamai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution 👍

Am seeing quite a few problems with this implementation so far:

Mismatch between selected Time Zones when opening the select back up
Screenshot 2025-01-30 at 09 27 23

Shouldn't the date selection clear when I clear the dashboard selection?
Screenshot 2025-01-30 at 09 46 35

Does the UI stipulate having to display the time zone along with the selection?
Screenshot 2025-01-30 at 09 46 35

Why is GMT enforced?

const [startDateTime, setStartDateTime] = useState<DateTime | null>(
startDateTimeValue
startDateTimeValue ??
DateTime.now().set({ second: 0 }).minus({ minutes: 30 })
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Has there been discussions around setting this to the user time zone?

@@ -249,6 +248,9 @@ export const DateTimePicker = ({
padding: 0,
}),
},
field: {
readOnly: true,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants